Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve not-a-function error message #50

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

gnclmorais
Copy link

Fixes #23. When the passed fn to the helper is not a function, this commit improves the error message users will see, hopefully helping with the debugging process.

First time contributing to an Ember project, so feel free to give me any feedback you have. Cheers!

When the passed `fn` to the helper is not a function, this commit
improves the error message users will see, hopefully helping with
the debugging process.

Fixes emberjs#23
@@ -84,4 +84,34 @@ module('Integration | Modifier | did-insert', function (hooks) {

assert.dom('.alert').hasClass('fade-in');
});

test('provides a useful error on insert', async function (assert) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gnclmorais nitpick, maybe name it provides a useful error when provided not a function?
Same for other tests.
Just so that mentally in future we may add some other "useful error on insert" for another use case

nonExistentMethod: undefined,
show: false,
});

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gnclmorais looks like this test fails. Di you try you add await settled() before setupOnerror() so that it re-renders and emit error and only after that resets?

Copy link
Author

@gnclmorais gnclmorais Oct 29, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’ve tried a couple of things to make sure this would work (your suggestion was one of them), but no luck so far… Curiously, it only fails with three older versions of Ember:

Scenario ember-lts-3.8: FAIL
Command run: ember test
┌────────────────────┬────────────────────┬──────────────────────────────┬──────────┐
│ Dependency         │ Expected           │ Used                         │ Type     │
├────────────────────┼────────────────────┼──────────────────────────────┼──────────┤
│ ember-source       │ ~3.8.0             │ 3.8.3                        │ yarn     │
└────────────────────┴────────────────────┴──────────────────────────────┴──────────┘

Scenario ember-lts-3.12: FAIL
Command run: ember test
┌────────────────────┬────────────────────┬──────────────────────────────┬──────────┐
│ Dependency         │ Expected           │ Used                         │ Type     │
├────────────────────┼────────────────────┼──────────────────────────────┼──────────┤
│ ember-source       │ ~3.12.0            │ 3.12.4                       │ yarn     │
└────────────────────┴────────────────────┴──────────────────────────────┴──────────┘

Scenario ember-lts-3.16: FAIL
Command run: ember test
┌────────────────────┬────────────────────┬──────────────────────────────┬──────────┐
│ Dependency         │ Expected           │ Used                         │ Type     │
├────────────────────┼────────────────────┼──────────────────────────────┼──────────┤
│ ember-source       │ ~3.16.0            │ 3.16.10                      │ yarn     │
└────────────────────┴────────────────────┴──────────────────────────────┴──────────┘

Do you think we could potentially run these tests on all versions except these?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

improve debugging possibilities of error messages
2 participants